Allow fully qualified paths for new secret name in secrets rename #417

CDeltakai · 2025-06-04T07:43:35Z

Description

This PR addresses a bug in the polykey secrets rename command where providing as a fully qualified path (e.g., MyVault:/NewSecret) would cause an ENOENT error. This occurred because the underlying rename operation expects a simple base name for the new secret, not a full path.

Issues Fixed

Fixes Secret Renaming 2nd Parameter Does not Recognize Fully Qualified Paths #393

Tasks

1. Change the CommandRename.ts script in src/secrets to allow for both simple base names someSecret and fully qualified paths vaultName:/someSecret

Final checklist

… new secret name and ensure its a valid base name even with the vaultName:secret syntax

…ts final new secret name

… in CommandRename

aryanjassal

The logic intuition is correct, but the implementation does not align with how it has been done in rest of the codebase. So, the implementation needs a bit of refactoring before it is ready for merging.

aryanjassal · 2025-06-09T23:58:37Z

src/secrets/CommandRename.ts

+
+  /**
+   * Processes the newSecretName argument to ensure it's a valid base name.
+   */
+  private static _processNewSecretNameArgument(
+    newSecretNameArg: string,
+    originalVaultName: string,
+  ): string {
+    let finalNewBaseName = newSecretNameArg;
+
+    if (newSecretNameArg.includes(':')) {
+      let parsedNewPath: Array<any>;
+      try {
+        parsedNewPath = binParsers.parseSecretPath(newSecretNameArg);
+      } catch (e: any) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALID: The new secret name '${newSecretNameArg}' appears to be a path but could not be parsed: ${e.message}`,
+        );
+      }
+      const newVaultNameInArg: string = parsedNewPath[0];
+      const newSecretPathPart: string = parsedNewPath[1];
+      if (newVaultNameInArg !== originalVaultName) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `ECROSSVAULT: Renaming to a different vault ('${newVaultNameInArg}') is not supported by this command. The target vault must be the same as the source vault ('${originalVaultName}').`,
+        );
+      }
+      if (
+        newSecretPathPart == null ||
+        newSecretPathPart.trim() === '' ||
+        newSecretPathPart.trim() === '/'
+      ) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALID: The path component of the new secret name '${newSecretNameArg}' is empty or invalid.`,
+        );
+      }
+      const parts = newSecretPathPart.split('/').filter((p) => p.length > 0);
+      if (parts.length === 0) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALID: Could not extract a valid base name from the path component '${newSecretPathPart}' in '${newSecretNameArg}'.`,
+        );
+      }
+      finalNewBaseName = parts[parts.length - 1];
+    } else {
+      if (newSecretNameArg.includes('/')) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALIDNAME: The new secret name '${newSecretNameArg}' must be a base name and cannot contain '/'. If you intended to specify a path, include the vault name (e.g., '${originalVaultName}:/path/NewName').`,
+        );
+      }
+    }
+    if (!finalNewBaseName || finalNewBaseName.trim() === '') {
+      throw new errors.ErrorPolykeyCLIRenameSecret(
+        `EEMPTYNAME: The new secret name derived from '${newSecretNameArg}' is empty.`,
+      );
+    }
+    if (finalNewBaseName.includes('/') || finalNewBaseName.includes(':')) {
+      throw new errors.ErrorPolykeyCLIRenameSecret(
+        `EINVALIDCHAR: The final new secret name '${finalNewBaseName}' (derived from '${newSecretNameArg}') is invalid. It cannot contain '/' or ':' characters.`,
+      );
+    }
+    return finalNewBaseName;
+  }


Polykey-CLI/src/utils/parsers.ts

Lines 77 to 116 in 9ae7ac1

// E.g. If 'vault1:a/b/c', ['vault1', 'a/b/c', undefined] is returned

// If 'vault1', ['vault1', undefined, undefined] is returned

// If 'vault1:abc=xyz', ['vault1', 'abc', 'xyz'] is returned

// If 'vault1:', an error is thrown

// If 'a/b/c', an error is thrown

// Splits out everything after an `=` separator

function parseSecretPath(inputPath: string): ParsedSecretPathValue {

// The colon character `:` is prohibited in vaultName, so it's first occurence

// means that this is the delimiter between vaultName and secretPath.

const colonIndex = inputPath.indexOf(':');

// If no colon exists, treat entire string as vault name

if (colonIndex === -1) {

return [parseVaultName(inputPath), undefined, undefined];

}

// Calculate vaultName and secretPath

const vaultNamePart = inputPath.substring(0, colonIndex);

const secretPathPart = inputPath.substring(colonIndex + 1);

// Calculate contents after the `=` separator (value)

const equalIndex = secretPathPart.indexOf('=');

// If `=` isn't found, then the entire secret path section is the actual path.

// Otherwise, split the path section by the index of `=`. First half is the

// actual secret part, and the second half is the value.

const secretPath =

equalIndex === -1

? secretPathPart

: secretPathPart.substring(0, equalIndex);

const value =

equalIndex !== -1 ? secretPathPart.substring(equalIndex + 1) : undefined;

// If secretPath exists but it doesn't pass the regex test, then the path is

// malformed.

if (secretPath != null && !secretPathRegex.test(secretPath)) {

throw new InvalidArgumentError(

`${inputPath} is not of the format <vaultName>[:<secretPath>][=<value>]`,

);

}

// We have already tested if the secretPath is valid, and we can return it

// as-is.

const vaultName = parseVaultName(vaultNamePart);

return [vaultName, secretPath, value];

}

This already exists in a utility file. This file takes in a string, and returns ['vaultName', 'secretPath', undefined] for a string like vaultName:secretPath.

Also, we should not create private static helpers like this. If we need a helper function for anything, it should go in the utilities.

Polykey-CLI/src/secrets/CommandEdit.ts

Line 17 in 9ae7ac1

this.argument(

In this case, actually, there exists an even simpler method for this splitting. You can directly attach a parser to an argument, and it will automatically run that parser over the argument, and return the result of that directly. This also exists in this file for the secretPath argument.

Polykey-CLI/src/secrets/CommandRename.ts

Line 14 in 9ae7ac1

this.argument(

aryanjassal · 2025-06-10T00:01:08Z

tests/secrets/rename.test.ts

-      `${vaultName}:${secretName}`,
-      newSecretName,
+      `${vaultName}:${oldSecretName}`,
+      newSecretBaseName, // Use the simple base name


We shouldn't need comments in the test, the code should be self-descriptive. The variable names here are kind of obscure, so they can be named to oldSecretName and newSecretName for clarity.

aryanjassal · 2025-06-10T00:02:02Z

tests/secrets/rename.test.ts

    ];
    const result = await testUtils.pkStdio(command, {
      env: { PK_PASSWORD: password },
      cwd: dataDir,
    });
    expect(result.exitCode).toBe(0);
+    expect(result.stderr).toBe(''); // Expect no errors


We don't test stderr for being blank, as it is also used for logging instead of strictly for errors, so if a harmless log message was printed, then this test would be incorrectly failing.

aryanjassal · 2025-06-10T00:03:14Z

tests/secrets/rename.test.ts

    await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {
      const list = await vaultOps.listSecrets(vault);
-      expect(list.sort()).toStrictEqual([newSecretName]);
+      expect(list.sort()).toStrictEqual([newSecretBaseName]);
+      expect(list).not.toContain(oldSecretName);


If the list is strictly equal to the new secret, then that means that it only has a singular file being the new one. The second check is redundant.

aryanjassal · 2025-06-10T00:06:48Z

tests/secrets/rename.test.ts

+      cwd: dataDir,
+    });
+    expect(result.exitCode).not.toBe(0);
+    expect(result.stderr).toInclude('EPERM'); // This is because secretPath[1] will be undefined


Using stderr like this is better, but this can be improved yet further. You only check the prefix of the message to be present in stderr. You should be checking the entire error string to be present.

aryanjassal · 2025-06-10T00:11:33Z

tests/secrets/rename.test.ts

+  // New test case for the fix
+  test('should rename secret when new name is a fully qualified path', async () => {
+    const vaultName = 'vaultFQRename' as VaultName; // Use a distinct vault name for the test
+    const vaultId = await polykeyAgent.vaultManager.createVault(vaultName);
+    const oldSecretName = 'oldSecretName';
+    const newSecretBaseName = 'newSecretNameByPath'; // The actual target base name
+    const secretContent = 'content for fully qualified path rename test';
+
+    // Add initial secret
+    await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {
+      await vaultOps.addSecret(vault, oldSecretName, secretContent);
+    });
+
+    // Construct the fully qualified path for the new secret name
+    // This format matches the problematic case: VaultName:/NewName
+    const newSecretFullyQualified = `${vaultName}:/${newSecretBaseName}`;
+
+    command = [
+      'secrets',
+      'rename',
+      '-np',
+      dataDir,
+      `${vaultName}:${oldSecretName}`, // Source secret path
+      newSecretFullyQualified, // New secret name as fully qualified path
+    ];
+
+    const result = await testUtils.pkStdio(command, {
+      env: { PK_PASSWORD: password },
+      cwd: dataDir,
+    });
+
+    expect(result.exitCode).toBe(0);
+    expect(result.stderr).toBe(''); // Expect no errors
+
+    // Verify the rename
+    await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {
+      const secretsList = await vaultOps.listSecrets(vault);
+      expect(secretsList).toContain(newSecretBaseName); // Check for the base name
+      expect(secretsList).not.toContain(oldSecretName);
+      expect(secretsList.length).toBe(1); // Assuming only this secret is in the test vault


The aim of the fix was to invalidate the previous approach of providing the new name instead of a full path.

$ polykey secrets rename vault:secret newname # wrong $ polykey secrets rename vault:secret vault:secret2 # correct

You should be able to merge those two tests into a single test like follows:

// ... const command1 = [ 'secrets', 'rename', 'vault:secret', 'newname' ]; const result1 = await testUtils.pkStdio(command1, /* ... */); expect(result1.exitCode).toBe(64); // Test didn't succeed because command was wrong const command2 = [ /* ... */, 'vault:secret', 'vault:newname' ]; const result2 = /* ... */; expect(result2.exitCode).toBe(0);

aryanjassal · 2025-06-10T00:12:41Z

src/secrets/CommandRename.ts

-      if (secretPath[1] == null) {
+      if (
+        secretPath[1] == null ||
+        secretPath[1].trim() === '' ||
+        secretPath[1].trim() === '/'
+      ) {
        throw new errors.ErrorPolykeyCLIRenameSecret(
          'EPERM: Cannot rename vault root',
        );
      }
+
+      // Process the newSecretNameArg using the abstracted helper method.
+      // secretPath[0] is the original vault name.
+      const finalNewSecretName = CommandRename._processNewSecretNameArgument(
+        newSecretNameArg,
+        secretPath[0],
+      );
+


The rename command cannot move files across vaults, just rename it within the same vault. So, you need to add a check that the vault name in both the parameters is the same, otherwise throw an error. This should also have an associated test with it.

CDeltakai added 3 commits June 4, 2025 17:12

feat: added helper method to CommandRename for secrets to process the…

1a4869c

… new secret name and ensure its a valid base name even with the vaultName:secret syntax

feat: made CommandRename take in the parsed new secret name arg for i…

3464d41

…ts final new secret name

fix: add type annotations for newVaultNameInArg and newSecretPathPart…

32e30ac

… in CommandRename

CDeltakai self-assigned this Jun 4, 2025

CDeltakai added 2 commits June 4, 2025 17:58

feat: added new test case for updated CommandRename for secrets

ee0b3e2

fix: lintfix

7e03a92

CDeltakai requested review from aryanjassal and tegefaulkes June 4, 2025 08:00

aryanjassal requested changes Jun 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow fully qualified paths for new secret name in secrets rename #417

Allow fully qualified paths for new secret name in secrets rename #417

CDeltakai commented Jun 4, 2025 •

edited

Loading

Uh oh!

aryanjassal left a comment

Uh oh!

aryanjassal Jun 9, 2025

Uh oh!

aryanjassal Jun 10, 2025

Uh oh!

aryanjassal Jun 10, 2025

Uh oh!

aryanjassal Jun 10, 2025

Uh oh!

aryanjassal Jun 10, 2025

Uh oh!

aryanjassal Jun 10, 2025

Uh oh!

aryanjassal Jun 10, 2025

Uh oh!

Uh oh!

	// E.g. If 'vault1:a/b/c', ['vault1', 'a/b/c', undefined] is returned
	// If 'vault1', ['vault1', undefined, undefined] is returned
	// If 'vault1:abc=xyz', ['vault1', 'abc', 'xyz'] is returned
	// If 'vault1:', an error is thrown
	// If 'a/b/c', an error is thrown
	// Splits out everything after an `=` separator
	function parseSecretPath(inputPath: string): ParsedSecretPathValue {
	// The colon character `:` is prohibited in vaultName, so it's first occurence
	// means that this is the delimiter between vaultName and secretPath.
	const colonIndex = inputPath.indexOf(':');
	// If no colon exists, treat entire string as vault name
	if (colonIndex === -1) {
	return [parseVaultName(inputPath), undefined, undefined];
	}
	// Calculate vaultName and secretPath
	const vaultNamePart = inputPath.substring(0, colonIndex);
	const secretPathPart = inputPath.substring(colonIndex + 1);
	// Calculate contents after the `=` separator (value)
	const equalIndex = secretPathPart.indexOf('=');
	// If `=` isn't found, then the entire secret path section is the actual path.
	// Otherwise, split the path section by the index of `=`. First half is the
	// actual secret part, and the second half is the value.
	const secretPath =
	equalIndex === -1
	? secretPathPart
	: secretPathPart.substring(0, equalIndex);
	const value =
	equalIndex !== -1 ? secretPathPart.substring(equalIndex + 1) : undefined;
	// If secretPath exists but it doesn't pass the regex test, then the path is
	// malformed.
	if (secretPath != null && !secretPathRegex.test(secretPath)) {
	throw new InvalidArgumentError(
	`${inputPath} is not of the format <vaultName>[:<secretPath>][=<value>]`,
	);
	}
	// We have already tested if the secretPath is valid, and we can return it
	// as-is.
	const vaultName = parseVaultName(vaultNamePart);
	return [vaultName, secretPath, value];
	}

Allow fully qualified paths for new secret name in secrets rename #417

Are you sure you want to change the base?

Allow fully qualified paths for new secret name in secrets rename #417

Conversation

CDeltakai commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Fixed

Tasks

Final checklist

Uh oh!

aryanjassal left a comment

Choose a reason for hiding this comment

Uh oh!

aryanjassal Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

aryanjassal Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

aryanjassal Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

aryanjassal Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

aryanjassal Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

aryanjassal Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

aryanjassal Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CDeltakai commented Jun 4, 2025 •

edited

Loading